Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try/typer reorg #47

Merged
merged 9 commits into from Mar 8, 2014
Merged

Try/typer reorg #47

merged 9 commits into from Mar 8, 2014

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 6, 2014

A rather sweeping commit of typer structure. Main goal: Avoid code duplication and possible divergence of logic between tpd and Typer.

I needed the previous fix/overloadedAccess pull request to make the code in this one compile under dotty. I removed that former pull request and combined the two, so that we can get this in quickly.

Review by @DarkDimius @samuelgruetter

@xeno-by
Copy link
Member

xeno-by commented Mar 6, 2014

There's a typo in the second-to-last commit title.

@xeno-by
Copy link
Member

xeno-by commented Mar 6, 2014

Also the description of the second-to-last commit has weird line breaks when rendered in Github. Seems that it's caused by the first line of the description being 110 characters long. Typical line length expected by Git-related tools is 80 characters.

@xeno-by
Copy link
Member

xeno-by commented Mar 6, 2014

There's also a typo in the second commit description.

@xeno-by
Copy link
Member

xeno-by commented Mar 6, 2014

Btw it's really cool to see these really fine-grained changes with all the explanations. Do all the tests pass for the individual commits?

@adriaanm
Copy link
Contributor

adriaanm commented Mar 6, 2014

+1 on enjoying the details in the commit messages

our guidelines for commit titles are: max 60 chars for the first line, should be written using active verbs, so that it makes sense as a bullet item in release notes (i.e., it's the answer to the question how this commit improves the codebase)

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2014

Thanks for the tips on commit formatting. Is there a way to do this in the existing commits (preferably on github itself), or do I have to create new ones?

@sjrd
Copy link
Member

sjrd commented Mar 7, 2014

There's no way on GitHub directly, AFAIK. With git you can of course simply do git commit --amend if it's only the HEAD, or git rebase -i master and change the picks to reword.

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2014

OK, I think I'll take in the advice for future commits, but won't change the present ones.

Rewrap needs to produce alternatives with signatures. Otgerwise the new denotation will simply overwrite the old because both the overloaded TermRef and the alternative will hash to the same unique TermRef.
Overloaded TermRefs do not have an info, and consequently do not support =:=. Yet in Typer#checkNewOrShadowed we compared termrefs with =:=. This gives an exception if the termrefs are overloaded. The fix is to provide a new method isSameRef in TypeComparer which is called instead of =:= in Typer#checkNewOrShadowed.
Goal is better modularization and avoiding code duplication and divergence between Typer and tpd. As a first step, we split Inferencing into Inferencing, Checking, and ProtoTypes. Inferencing and Checking become Typer traits, while ProtoTypes remains a global object.

Eventually:

 - we want to have a SimpleTyper, which does the main stuff in tpd, and can mixin either full checking or no checking. Each method in SimpleTyper takes an untyped tree (which is assumed to have typed arguments) and adds a toplevel type to that tree. The methods subsume the type-checking parts in Typers, except for
(1) simplifications and expansions (2) computing prototypes and recursing with them into childtrees (3) adaptation. The method calls the necessary checking operations, which may however be stubbed out.

The idea is already exercised in the typechecking code for Literal and New, except that for now it calls methods in tpd (they will become methods in SimpleTyper instead).

 - Typer should inherit from SimpleTyper, and forward all logic except for (1) - (3) to it.

 - tpd should call the simple typer it gets from ctx.typer

 - ctx.typer should be a SimpleTyper, not a complete one.
TypeAssigners assign a toplevel type to a node. They are mixed into Typer, and can be accessed from tpd using
ctx.typeAssigner.
Common code between tpd and Typer has been factored out into class TypeAssigner.
@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2014

Rebased all commits on current master.

val qual1 = followTypeAlias(qual)
if (qual1.isEmpty) notAnExtractor(sel)
else trySelectUnapply(qual1)(_ => notAnExtractor(sel))
}

def fromScala2x = unapply.symbol.exists && (unapply.symbol.owner is Scala2x)
def fromScala2x = unapplyFn.symbol.exists && (unapplyFn.symbol.owner is Scala2x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"fromScala2x" feels like a conversion, maybe "isFromScala2x"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@DarkDimius
Copy link
Member

LGTM

odersky added a commit that referenced this pull request Mar 8, 2014
@odersky odersky merged commit d827b01 into scala:master Mar 8, 2014
@odersky odersky deleted the try/typer-reorg branch March 8, 2014 18:07
noti0na1 added a commit to noti0na1/dotty that referenced this pull request Dec 5, 2019
Implement flow typing using nullability check; Update documents
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants